-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use terms aggs instead of composite refactor of Authentications Table based on feedback #29283
Use terms aggs instead of composite refactor of Authentications Table based on feedback #29283
Conversation
Pinging @elastic/secops |
This comment has been minimized.
This comment has been minimized.
x-pack/plugins/secops/public/components/page/hosts/authentications_table/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/secops/public/components/page/hosts/authentications_table/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/secops/public/components/page/hosts/authentications_table/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/secops/public/components/page/hosts/authentications_table/index.tsx
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
* Made it sort by desc of number of failures * Reduced type usages where possible to Ecs based types * Added newer fields to the Authentications based on feedback
1972f3f
to
5d8917d
Compare
This comment has been minimized.
This comment has been minimized.
@@ -204,7 +205,7 @@ describe('Flyout', () => { | |||
}); | |||
|
|||
test('should call the onOpen when the mouse is clicked for rendering', () => { | |||
const showTimeline = jest.fn(); | |||
const showTimeline = (jest.fn() as unknown) as ActionCreator<{ id: string; show: boolean }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: with stricter types using ActionCreator
, jest.fn()
needs me to use this ugly casting to work? If there's another way I would like to know.
@@ -234,7 +235,7 @@ describe('Flyout', () => { | |||
const stateShowIsTrue = set('local.timeline.timelineById.test.show', true, state); | |||
const storeShowIsTrue = createStore(stateShowIsTrue); | |||
|
|||
const showTimeline = jest.fn(); | |||
const showTimeline = (jest.fn() as unknown) as ActionCreator<{ id: string; show: boolean }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: with stricter types using ActionCreator
, jest.fn()
needs me to use this ugly casting to work? If there's another way I would like to know.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
}, | ||
queryDate: { | ||
from: startDate, | ||
to: moment().valueOf(), | ||
to: Date.now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this a few other places (like in HostsTable
), but what's the intent of using the current time for queryDate.to
? Why would this not be the to
value of the GlobalTime
component that sets the time constraints for the widget itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should probably be constrained to the global date time. The Date.now()
kind of shows the bug more clearly it looks like.
<DraggableWrapper | ||
dataProvider={{ | ||
and: [], | ||
enabled: true, | ||
id: node._id!, | ||
id: escapeDataProviderId(`events-table-${node._id!}-hostName-${hostName}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much appreciated!
<DraggableWrapper | ||
dataProvider={{ | ||
and: [], | ||
enabled: true, | ||
excluded: false, | ||
id: node._id!, | ||
id: escapeDataProviderId(`hosts-table-${node._id!}-hostName-${hostName}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional query capabilities in the form of draggable content to the timeline, fixes to make provider IDs globally unique, and type safety in this PR are all appreciated @FrankHassanabad !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing Andrew -- the type cleanup (especially in regards to ActionCreator
) is greatly appreciated. As well with the introduction of escapeDataProviderId()
for dataprovider id generation -- will use this going forward. LGTM!
extendMap
for ECS mapping fields to make things easierit(
in favor oftest(
escapeDataProviderId
and consolidated code to use thatActionCreator
Edges
in the code to remove less boilerplatemoment().valueOf()
forDate.now()
as it's more semantic and we can remove moment where that's all we were using it for.After:
After in dark mode where other columns are draggable:
Before: